Support xz tarballs if available
#823
Conversation
|
(fwiw, not very happy with the DRY-ness of these checks but I followed the convention of not polluting the global scope) |
|
Not sure why the test case wasn't executed. Edit: most likely because I didn't even push it.... 😴 |
jbergstroem
force-pushed
the
feature/prefer-xz
branch
2 times, most recently
from
501731d
to
3ce5a32
Compare
|
|
||
|
|
||
| OLDPATH=$PATH | ||
| export PATH=../../xz-test |
There was a problem hiding this comment.
this will likely break things in tests - could you preserve the existing path, and prepend your new dir, rather than overwriting it entirely?
There was a problem hiding this comment.
I previously had export PATH=$OLDPATH but since every test seems sourced separately I didn't see anything breaking. Happy to re-add? If I prepend dir we will very likely run into false positives since most machines we will be testing on already has xz installed.
There was a problem hiding this comment.
Overwriting PATH means that any nvm internals relying on things that should be available via PATH will break - doing it in tests makes future refactoring of nvm more brittle, and doesn't accurately represent the user's environment.
I think that for the tests where zx is not installed - which ideally are in a separate test file from the ones testing that it is installed - should make whatever assertions or stubs are needed to cover that case.
There was a problem hiding this comment.
Ok. Would juggling path (OLDPATH, PATH) inside of one file be good enough? Having two files would sort of break your unit test convention file layout.
There was a problem hiding this comment.
I don't understand that part - our convention is as close to one test per file as possible. Perhaps you want to make a subdirectory for xz, and in that, use setup and teardown files, for the common code?
ljharb
added
installing node
| @@ -1999,6 +2031,10 @@ nvm_supports_source_options() { | |||
| [ "_$(echo 'echo $1' | . /dev/stdin yes 2> /dev/null)" = "_yes" ] | |||
| } | |||
|
|
|||
| nvm_supports_xz() { | |||
| [ $(command which xz) ] && nvm_is_merged_node_version "$1" | |||
There was a problem hiding this comment.
instead of a subshell, could this just be command which xz 2>&1 >/dev/null && nvm_is_merged_node_version "$1"?
|
Just following up here. I added stuff to |
|
The latest change should bail early and move the cleanup phase to Edit: ah, part of urchin. |
| @@ -1999,6 +2015,10 @@ nvm_supports_source_options() { | |||
| [ "_$(echo 'echo $1' | . /dev/stdin yes 2> /dev/null)" = "_yes" ] | |||
| } | |||
|
|
|||
| nvm_supports_xz() { | |||
| command which xz 2>&1 >/dev/null && nvm_is_merged_node_version "$1" | |||
There was a problem hiding this comment.
can we make this return true for the appropriate io.js versions also?
There was a problem hiding this comment.
Sure. This is bit of a mixed bag though. We messed up during release phase, so xz tarballs for darwin is 2.3.2 and up whereas linux/src is 1.0.0 and up. Go with the safe option of 2.3.2 and up?
jbergstroem
force-pushed
the
feature/prefer-xz
branch
3 times, most recently
from
0661c9d
to
c360313
Compare
Saves us ~25% bandwidth while downloading the payload. This only applies to hosts that has the `xz` binary and attempts to use iojs 2.3.2 or newer (this includes nodejs 4.0+ as well). Older targets are unaffected.
jbergstroem
force-pushed
the
feature/prefer-xz
branch
from
c360313
to
b8e4917
Compare
Support `xz` tarballs if available (on io.js >= 2.3.2 and node >= 4)
Saves us ~25% bandwidth, but only nodejs 4.0.0 and forward. We check for the existence of
xzso older versions or targets without it is unaffected.